-
Notifications
You must be signed in to change notification settings - Fork 292
Short circuit non-tagged unions #430
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
CodSpeed Performance ReportMerging #430 Summary
Benchmarks breakdown
|
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## main #430 +/- ##
=======================================
Coverage 95.50% 95.50%
=======================================
Files 90 90
Lines 10735 10761 +26
Branches 10 10
=======================================
+ Hits 10252 10277 +25
- Misses 482 483 +1
Partials 1 1
Continue to review full report in Codecov by Sentry.
|
@art049 I just added a benchmark here. It would be really cool if I could hit a button and say "run these benchmarks on |
if !extra.exhaustive { | ||
return Err(ValError::Omit); | ||
} | ||
errors.extend(line_errors.into_iter().map(|err| err.with_outer_location(index.into()))); | ||
} | ||
Err(ValError::Omit) => (), | ||
Err(ValError::Omit) => { | ||
if !extra.exhaustive { | ||
return Err(ValError::Omit); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just an example, I think we'd have to implement this in other places, wherever it makes sense.
We should figure out at build time if exhaustiveness applies to the child validators. E.g. a |
For what it's worth @adriangb, might make sense to make a separate PR to main to add this benchmark to make the before/after more obvious. |
There's 2 cons to that:
|
From discussion today: this is an interesting idea, but we have to:
|
Super interesting @adriangb, I think this could fit well with the historical run feature(basically running the latest benches on older revisions). It might just be a bit complicated regarding dependency management though. But I'll think about it when I work on it! |
Closing this PR for now |
The idea here is to do a first pass where we do not do exhaustive validation and instead try to optimistically find a match. Then if we don't find one we go back and do exhaustive validation so that we can build up the errors and such. In the example benchmark (which I know is a list but I think is representative of more complex objects) this is a 2x speedup (and it would be a 100x speedup if there were 100 options in the union).
The penalty of this is the same as doing the strict mode pass first (although I understand there's non-performance reasons to do that): we have to re-validate in an exhaustive match later. I think it would be interesting to explore some sort of "state cache" in the future that can be used for this as well as the strict mode first pass so that a validator can "pick up where it left off". I think the easiest way to do this would be to convert:
Into something like:
And then each Validator can come up with it's own state to store if it makes sense for it. I think the only ones that would make sense are lists and typed-dict where it'd be trivial to store "the last field/index that passed validation" or something like that.